Initial build-using-dockerfile implementation by nalind · Pull Request #59 · containers/buildah

nalind · 2017-03-29T19:44:39Z

Add a build-using-dockerfile command which uses openshift/imagebuilder to wrap parsing and handle most of the dispatching, and the logic behind the other subcommands to implement assorted instructions.

nalind · 2017-03-29T20:05:21Z

(8) [Buildah] Add "dockah build-using-dockerfile" aka "buildah bud"

rhatdan · 2017-03-29T19:57:14Z

cmd/buildah/build.go

@@ -0,0 +1,136 @@
+package main


Should we call this build-using-dockerfile.go for consistency?
Or bud.go?

bud's a lot less typing, so I'll rename it to that.

rhatdan · 2017-03-29T19:57:35Z

cmd/buildah/build.go

+var (
+	buildFlags = []cli.Flag{
+		cli.BoolFlag{
+			Name:  "quiet",


rhatdan · 2017-03-29T19:58:18Z

cmd/buildah/build.go

+			Usage: "prefix to prepend to the image name in order to pull the image",
+			Value: DefaultRegistry,
+		},
+		cli.BoolTFlag{


Docker does this by default. I don't think we need an alternative.

rhatdan · 2017-03-29T19:58:49Z

cmd/buildah/build.go

+			Name:  "pull",
+			Usage: "pull the image if not present",
+		},
+		cli.BoolFlag{


I don't think we should do this either, just do what docker does.

This defaults to true, and --pull-always defaults to false, like "buildah from" does. I'll note that "docker build"'s --pull is equivalent to --pull-always, so maybe we need to drop --pull from both, and rename --pull-always to --pull?

rhatdan · 2017-03-29T19:59:13Z

cmd/buildah/build.go

+		},
+		cli.StringSliceFlag{
+			Name:  "arg",
+			Usage: "`argument=value` to supply to the builder",


What is an example of this?

This is used to supply a value for a variable named by an ARG instruction in a Dockerfile, after which the variable's value can be referred to in other instructions, and will be added to the environment list we get when we're told to process a RUN instruction. Renaming this to --build-arg.

rhatdan · 2017-03-29T20:05:58Z

cmd/buildah/build.go

+		Output:              output,
+	}
+
+	return imagebuildah.BuildDockerfileFromFile(store, options, dockerfiles...)


Should this just be BuildDockerfile(...)

rhatdan · 2017-03-29T20:08:33Z

imagebuildah/build.go

+	systemContext                  *types.SystemContext
+}
+
+func getSystemContext(signaturePolicyPath string) *types.SystemContext {


Should this function be just
systemContext(...)

fwiw, I prefer keeping the get in the name here.

I am fine with get, but I think @mrunalp pointed out that there is some kind of GOLANG standard that says this is discouraged. I think golint and goverify used in CRI-O and runc give you errors when you start functions with get.

Here is the suggested style reference: https://golang.org/doc/effective_go.html#Getters

Hmm, that's focused more on wrappers for reading and writing object members, but in terms of what it does, it probably makes more sense to call it makeSystemContext().

rhatdan · 2017-03-29T20:09:49Z

imagebuildah/build.go

+	return sc
+}
+
+func (b *buildahExecutor) Preserve(path string) error {


We need to add comments to all externally defined functions. We need to start running golint and govalidate against our code. I have no idea what Preserve does and it seems to be not implemented yet.

See #62 for adding formatting and style checking to CI. The Preserve method is used to signal the Executor that the path has been marked as part of a volume, which means we need to keep instructions other than an explicit ADD or COPY from modifying anything below that point. The imagebuilder dockerclient implementation uses caching/restoring around its handling of RUN instructions to accomplish this. Adding that.

rhatdan · 2017-03-29T20:10:07Z

imagebuildah/build.go

+	return nil
+}
+
+func (b *buildahExecutor) Copy(excludes []string, copies ...imagebuilder.Copy) error {


rhatdan · 2017-03-29T20:10:43Z

imagebuildah/build.go

+	return nil
+}
+
+func (b *buildahExecutor) Run(run imagebuilder.Run, config docker.Config) error {


rhatdan · 2017-03-29T20:14:13Z

BTW This is pretty awesome.

TomSweeneyRedHat · 2017-03-30T14:45:02Z

imagebuildah/build.go

+}
+
+// BuildDockerfile builds an image using instructions from the named file.
+func BuildDockerfileFromFile(store storage.Store, options BuildDockerfileOptions, dockerfile ...string) error {


If you change the name to BuildDockerfileFromFile in build.go, you probably want to do the same here.

nalind · 2017-04-05T14:24:15Z

Updated:

rebased
golint clean
tests no longer fail without Broaden what "rmi" can accept, and add tests #61
Preserve() is now implemented, so handling of volumes during bud should be more in line with expectations
the bud command's --arg flag is now named --build-arg
the bud command's -t, -f, and -q flags now also have long names --tag, --file, and --quiet
Dockerfiles can be specified using http/https URLs
context directories can be specified as "git://" URLs, "github.com/..." repositories, or as http/https URLs of archives
handling of multiple Dockerfiles is more in line with how openshift/imagebuildah uses them (in order, only the first FROM: counts)
renamed getSystemContext() to makeSystemContext()
more tests

rhatdan · 2017-04-08T20:14:17Z

LGTM

Do we eventually want to turn ImageBuilder to use Buildah? Might want to move this code out of ImageBuilder into Buildah at that point.

nalind · 2017-04-10T13:58:24Z

Moving the tell-Travis-to-stop-using-1.6 patch to #65 and rebasing before merging.

nalind · 2017-04-10T15:06:55Z

I think the imagebuilder CLI could be taught to toggle between its current dockerclient Executor and the one we implement here, as @smarterclayton suggested in openshift/imagebuilder#33. We don't yet support all of the options and hooks that the imagebuilder CLI tool wants to be able to specify, but that's fixable.

smarterclayton · 2017-04-10T21:39:32Z

You should also be able to snip more of the dependency tree that we pull in - let me know if there are changes that need to be done to make the core imagebuilder code not depend on docker dependencies.

rhatdan · 2017-04-12T21:28:02Z

cmd/buildah/bud.go

+	budFlags = []cli.Flag{
+		cli.BoolFlag{
+			Name:  "quiet, q",
+			Usage: "refrain from announcing build instructions",


Should we reverse this and change it to verbose, and then default it to value so we don't announce build instructions?

I'm not strongly opposed to it, but docker build and imagebuilder output this info by default.

rh-atomic-bot · 2017-04-12T21:31:25Z

☔ The latest upstream changes (presumably e67d2e7) made this pull request unmergeable. Please resolve the merge conflicts.

rhatdan · 2017-04-12T21:31:48Z

man pages and bash completions?

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

Vendor openshift/imagebuilder and its dependencies, and pick up new dependencies on golang.org/x/text and github.com/opencontainers/selinux. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

Add a build-using-dockerfile command (alias: bud) which uses openshift/imagebuilder to wrap parsing and dispatching, and runc (or another OCI runtime) to handle RUN instructions. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

It's nowhere near exhaustive, but it's a start. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

Add completion for build-using-dockerfile and our renaming of the containers/images --no-truncate flag to --notruncate during the CLI overhaul. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

nalind · 2017-04-13T20:48:41Z

Added a man page and updated the bash-completion script.

rhatdan · 2017-04-13T21:40:56Z

docs/buildah-bud.md

+buildah bud - Build an image using instructions from Dockerfiles.
+
+## SYNOPSIS
+**buildah** **bud | build-using-dockerfile** [*options* [...]] [**context**]


Is the context directory optional?

I guess it is if you specify a Dockerfile.

If at least one of the Dockerfiles is a local file, we assume the directory that it's in is the context directory.

rhatdan · 2017-04-13T21:42:49Z

@rh-atomic-bot r+ 390defd

rh-atomic-bot · 2017-04-13T21:42:53Z

⌛ Testing commit 390defd with merge d410ce9...

Vendor openshift/imagebuilder and its dependencies, and pick up new dependencies on golang.org/x/text and github.com/opencontainers/selinux. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com> Closes: #59 Approved by: rhatdan

Add a build-using-dockerfile command (alias: bud) which uses openshift/imagebuilder to wrap parsing and dispatching, and runc (or another OCI runtime) to handle RUN instructions. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com> Closes: #59 Approved by: rhatdan

It's nowhere near exhaustive, but it's a start. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com> Closes: #59 Approved by: rhatdan

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com> Closes: #59 Approved by: rhatdan

Add completion for build-using-dockerfile and our renaming of the containers/images --no-truncate flag to --notruncate during the CLI overhaul. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com> Closes: #59 Approved by: rhatdan

rh-atomic-bot · 2017-04-14T01:25:56Z

☀️ Test successful - status-redhatci
Approved by: rhatdan
Pushing d410ce9 to master...

Signed-off-by: Urvashi Mohnani <umohnani@redhat.com> Closes: #59 Approved by: rhatdan

nalind force-pushed the imagebuildah branch from e6c9e97 to b3deab9 Compare March 29, 2017 20:02

rhatdan reviewed Mar 29, 2017

View reviewed changes

nalind mentioned this pull request Mar 29, 2017

Broaden what "rmi" can accept, and add tests #61

Closed

TomSweeneyRedHat reviewed Mar 30, 2017

View reviewed changes

nalind force-pushed the imagebuildah branch 10 times, most recently from dda9371 to d664cbf Compare April 5, 2017 14:10

nalind mentioned this pull request Apr 10, 2017

Maybe use openshift/imagebuilder #7

Closed

nalind force-pushed the imagebuildah branch from d664cbf to ede3a67 Compare April 10, 2017 13:59

nalind force-pushed the imagebuildah branch 3 times, most recently from 91dbef9 to 2202a41 Compare April 12, 2017 21:24

rhatdan reviewed Apr 12, 2017

View reviewed changes

Use imagebuilder's definition of "scratch"

33002c2

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

nalind added 3 commits April 12, 2017 17:32

Vendor openshift/imagebuilder

62ca12b

Vendor openshift/imagebuilder and its dependencies, and pick up new dependencies on golang.org/x/text and github.com/opencontainers/selinux. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

Add a build-using-dockerfile command

6e50133

Add a build-using-dockerfile command (alias: bud) which uses openshift/imagebuilder to wrap parsing and dispatching, and runc (or another OCI runtime) to handle RUN instructions. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

Add a few tests for building images

eff24d5

It's nowhere near exhaustive, but it's a start. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

nalind force-pushed the imagebuildah branch from 2202a41 to eff24d5 Compare April 12, 2017 21:35

Add a man page for buildah-bud(1)

f3d0a9d

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

nalind force-pushed the imagebuildah branch from 627ccc3 to f3d0a9d Compare April 13, 2017 20:33

Update bash-completions for build-using-dockerfile

390defd

Add completion for build-using-dockerfile and our renaming of the containers/images --no-truncate flag to --notruncate during the CLI overhaul. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

rhatdan reviewed Apr 13, 2017

View reviewed changes

rh-atomic-bot pushed a commit that referenced this pull request Apr 13, 2017

Add a few tests for building images

7615970

It's nowhere near exhaustive, but it's a start. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com> Closes: #59 Approved by: rhatdan

rh-atomic-bot pushed a commit that referenced this pull request Apr 13, 2017

Add a man page for buildah-bud(1)

31b6e7b

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com> Closes: #59 Approved by: rhatdan

rh-atomic-bot closed this in bed7525 Apr 14, 2017

nalind deleted the imagebuildah branch May 11, 2017 19:09

nalind pushed a commit that referenced this pull request Nov 28, 2017

Update kpod export to use the new container state and runtime

2a3934f

Signed-off-by: Urvashi Mohnani <umohnani@redhat.com> Closes: #59 Approved by: rhatdan

github-actions bot added the locked - please file new issue/PR label Nov 1, 2023

github-actions bot locked as resolved and limited conversation to collaborators Nov 1, 2023

Conversation

nalind commented Mar 29, 2017

Uh oh!

nalind commented Mar 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhatdan commented Mar 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nalind commented Apr 5, 2017

Uh oh!

rhatdan commented Apr 8, 2017

Uh oh!

nalind commented Apr 10, 2017

Uh oh!

nalind commented Apr 10, 2017

Uh oh!

smarterclayton commented Apr 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rh-atomic-bot commented Apr 12, 2017

Uh oh!

rhatdan commented Apr 12, 2017

Uh oh!

nalind commented Apr 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhatdan commented Apr 13, 2017

Uh oh!

rh-atomic-bot commented Apr 13, 2017

Uh oh!

rh-atomic-bot commented Apr 14, 2017